Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Kubernetes: --max-pods=30 should be Azure CNI-only #3543

Merged
merged 5 commits into from
Jul 25, 2018

Conversation

jackfrancis
Copy link
Member

What this PR does / why we need it: Instead of defaulting to --max-pods to 30 (the exceptional case) and then overriding to 110 (the more general case), it's easier to set to 110 by default and override to 30 when Azure CNI.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

If applicable:

  • documentation
  • unit tests
  • tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

Kubernetes: --max-pods=30 should be Azure CNI-only

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@acs-bot
Copy link

acs-bot commented Jul 24, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ghost ghost assigned jackfrancis Jul 24, 2018
@ghost ghost added in progress and removed approved labels Jul 24, 2018
@donaldguy
Copy link

This logic should probably also be reflected in

if agentPoolProfile.MaxPods == nil {
// default is kubenet
if networkProfile == nil || networkProfile.NetworkPlugin == v20180331.Kubenet {
maxPods = DefaultKubernetesMaxPodsKubenet
} else {
maxPods = DefaultKubernetesMaxPodsAzureCNI
}
} else {
maxPods = strconv.Itoa(*agentPoolProfile.MaxPods)
}
?

@codecov
Copy link

codecov bot commented Jul 24, 2018

Codecov Report

Merging #3543 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3543      +/-   ##
==========================================
+ Coverage   55.48%   55.48%   +<.01%     
==========================================
  Files         105      105              
  Lines       16047    16048       +1     
==========================================
+ Hits         8903     8904       +1     
  Misses       6393     6393              
  Partials      751      751

@acs-bot acs-bot added size/S and removed size/XS labels Jul 24, 2018
@acs-bot acs-bot added size/M and removed size/S labels Jul 25, 2018
@jackfrancis
Copy link
Member Author

Thank you @donaldguy, I agree!

@JunSun17 @weinong FYI

if networkProfile == nil || networkProfile.NetworkPlugin == v20180331.Kubenet {
maxPods = DefaultKubernetesMaxPodsKubenet
} else {
if networkProfile.NetworkPlugin == v20180331.Azure {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change to networkProfile != nil && networkProfile.NetworkPlugin == v20180331.Azure, to avoid npe.

Copy link
Collaborator

@JunSun17 JunSun17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@jackfrancis jackfrancis merged commit 50c5b9f into Azure:master Jul 25, 2018
@ghost ghost removed the in progress label Jul 25, 2018
@jackfrancis jackfrancis deleted the azure-cni-max-pods-30 branch July 25, 2018 17:28
PaulCharlton added a commit to ElementAnalytics/acs-engine that referenced this pull request Jul 26, 2018
* 'master' of https://github.com/Azure/acs-engine: (59 commits)
  Docs: Update user guide list to include Windows, update description of clusters (Azure#3473)
  update to Azure CNI v1.0.10 (Azure#3551)
  Adding 'make dev' equivalent for Windows (Azure#3471)
  print out ubuntu ver in e2e (Azure#3555)
  fix an issue where networkPlugin was not defined correctly when using calico or cilium (Azure#3271)
  Bump ginkgo to a tagged release (Azure#3554)
  Reenable AzureFile tests for Windows on K8s 1.11.1, resolves Azure#3439 (Azure#3496)
  removing rbac error checking from merge fn (Azure#3530)
  Change dns healthcheck to look at external domain (Azure#3282)
  DOCUMENTATION: Fix Documented Default Value for clusterSubnet (Azure#3474)
  Document required manual calico 2.6.3 -> calico 3.1.1 upgrade when upgrading from < 0.17.0-provisioned clusters (Azure#3208)
  revert --image-pull-policy=IfNotPresent for win (Azure#3553)
  --image-pull-policy=IfNotPresent for kubectl run commands (Azure#3552)
  Kubernetes: --max-pods=30 should be Azure CNI-only (Azure#3543)
  disable Azure CNI network monitor addon default (Azure#3550)
  only do az vm list for k8s (Azure#3540)
  Retire Swarm E2E for PR test coverage (Azure#3539)
  retire Azure CDN for container image repository proxying (Azure#3535)
  removed datadisk to allow scale after upgrade (Azure#3482)
  Pump k8s-azure-kms version (Azure#3531)
  ...
PaulCharlton added a commit to ElementAnalytics/acs-engine that referenced this pull request Jul 26, 2018
* master: (59 commits)
  Docs: Update user guide list to include Windows, update description of clusters (Azure#3473)
  update to Azure CNI v1.0.10 (Azure#3551)
  Adding 'make dev' equivalent for Windows (Azure#3471)
  print out ubuntu ver in e2e (Azure#3555)
  fix an issue where networkPlugin was not defined correctly when using calico or cilium (Azure#3271)
  Bump ginkgo to a tagged release (Azure#3554)
  Reenable AzureFile tests for Windows on K8s 1.11.1, resolves Azure#3439 (Azure#3496)
  removing rbac error checking from merge fn (Azure#3530)
  Change dns healthcheck to look at external domain (Azure#3282)
  DOCUMENTATION: Fix Documented Default Value for clusterSubnet (Azure#3474)
  Document required manual calico 2.6.3 -> calico 3.1.1 upgrade when upgrading from < 0.17.0-provisioned clusters (Azure#3208)
  revert --image-pull-policy=IfNotPresent for win (Azure#3553)
  --image-pull-policy=IfNotPresent for kubectl run commands (Azure#3552)
  Kubernetes: --max-pods=30 should be Azure CNI-only (Azure#3543)
  disable Azure CNI network monitor addon default (Azure#3550)
  only do az vm list for k8s (Azure#3540)
  Retire Swarm E2E for PR test coverage (Azure#3539)
  retire Azure CDN for container image repository proxying (Azure#3535)
  removed datadisk to allow scale after upgrade (Azure#3482)
  Pump k8s-azure-kms version (Azure#3531)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants